Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix running 'wheelhouse' test script on Travis CI #168

Closed
wants to merge 3 commits into from

Conversation

takluyver
Copy link
Member

This is an allowed failure, but so long as it's there, it might as well work.

@takluyver
Copy link
Member Author

@njsmith: this change would have the CI hitting your vorpus.org server - the code was there before, but commented out. I'd like your approval before merging it.

@rmcgibbo: your 'wheelhouse' at stanford.edu is no longer available. Let me know if you have the same files at some other URL. I don't think this is high priority, so don't spend energy on it, but if it's just a case of tweaking the URL, we can do that.

@rmcgibbo
Copy link
Member

I don't have those files anymore (Stanford took down my webspace a couple months after I graduated), but they weren't anything special.

@rmcgibbo
Copy link
Member

@takluyver: from these these travis logs from 3 years ago, it looks to me like the tests were covering wheels from lxml-3.7.3, Pillow-4.1.1, PyYAML-3.12.

@takluyver
Copy link
Member Author

Thanks! Do you think it's worth re-creating some more tests with real-world wheels, or is it more a nice-to-have alongside the main test suite? I wanted to make sure that whatever's there is running effectively, but I don't know how important it is.

# Download some more wheels that Robert made
URL=http://stanford.edu/~rmcgibbo/wheelhouse/
curl --silent $URL | grep 'a href' | grep whl | cut -d ' ' -f 8 | cut -d '=' -f 2 | cut -d '"' -f 2 | xargs -n1 -I '{}' wget -P wheelhouse-rmcgibbo "$URL/{}" -nc
URL=https://vorpus.org/~njs/tmp/manylinux-test-wheels/original/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not include these wheels in the source repo instead? We already have a few under the tests directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't want to clutter up the source repo with various multi-MB wheels; large binary files are a bit awkward to handle in git. This test can also easily be expanded to many more wheels, e.g. I saw this scikit-learn wheelhouse linked from #86.

Copy link
Member

@mayeut mayeut Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way not to clutter up the source repo with various multi-MB wheels would be to use git-lfs.

@lkollar
Copy link
Contributor

lkollar commented Jun 16, 2019

This should fix #142. We should also remove the allow_failures bit in .travis.yml.

@takluyver
Copy link
Member Author

The latest commit removes the allow_failures section.

I'm in two minds about this. On the one hand, it could be a flaky test, as it relies on downloading extra files. But then I don't think allow_failures tests are much use. At least the way I use Travis, I usually ignore them, and as these have been failing for about 18 months, it seems I'm not the only one.

In any case, I'd still like an OK from @njsmith before we merge this, because it will hit his server.

@mayeut
Copy link
Member

mayeut commented Jun 18, 2019

Do you think it's worth re-creating some more tests with real-world wheels, or is it more a nice-to-have alongside the main test suite? I wanted to make sure that whatever's there is running effectively, but I don't know how important it is.

Well, I think that's the main question here. I don't know if it's important either... Were those tests added to cover a feature, bug, in the past, I can't say looking at the history...

@rmcgibbo
Copy link
Member

I added those tests, but it was 3 years ago so I don't remember my exact train of thought. Generally though, at that time this project was something I was just hacking on. I don't think it was an official pypa project, I don't think that the first manylinux pep had yet been merged (it was probably under discussion), and it wasn't clear whether this software would even fill the intended use case. There wasn't really a "main test suite". I think I basically figured "if I can process some random wheels that I found on the internet, then it's probably working, and that's easier than investing in writing a bunch of unit tests right now"

So... real-world tests are always great. But I'd recommend making the decision on whether or not to include tests like this based on your current assessment of the coverage of the test suite, not some no-longer-applicable-determination I made three years ago when this was a solo hobby project that I was hacking on while procrastinating on my dissertation.

@takluyver
Copy link
Member Author

Thanks @rmcgibbo. That's the sort of scenario I would have guessed that script came from. :-)

@lkollar is doing some good work towards increasing test coverage. But I don't yet have a good feel for how well the test suite exercises gnarly cases that might come up in real-world wheels.

@lkollar
Copy link
Contributor

lkollar commented Jun 26, 2019

Given what @rmcgibbo said it might not be useful to try to fix these tests with the same wheels. The test-wheelhouse.sh script does not try to import the repaired wheels either, so this test is not that useful anyway. The repair step's return value is not even checked so we won't catch issues there. We now have several more exhaustive tests which validate specific features and actually test that the resulting libraries are usable. So I think we should consider two alternatives here:

If we keep the wheel I still think that we should store them in the source repo. pip does this too and it seems to be working just fine.

@takluyver
Copy link
Member Author

Now we've got coverage measurement set up and we can see we're exercising most of the code already, I think I'm comfortable with removing the wheelhouses script, and #179 does that.

@takluyver
Copy link
Member Author

#179 has been merged instead of this.

@takluyver takluyver closed this Jul 10, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants